DataOffload: Field offload of driver regions #457
Conversation
|
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/457/index.html |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #457 +/- ##
=======================================
Coverage 93.29% 93.29%
=======================================
Files 221 222 +1
Lines 41360 41387 +27
=======================================
+ Hits 38587 38613 +26
- Misses 2773 2774 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
It's very nice to get these extensions of the field offload capabilities. It makes a lot of sense to separate the field utilities and offload transformations like this and I think it's very nice to have the offload map tracking the offload variables like this👍.
|
Hi @wertysas , thanks for the review. I believe I've addressed all comments now (top two commits). Please have another look and let me know if you're happy. I'll then rebase on latest main for final integration. |
mlange05
force-pushed
the
naml-field-offload-regions
branch
from
4acdc97 to
82431ab
Compare
Sets are unordered, so using them to filter creates an arbitrary ordering, which in turn yields an unpredictable order of declarations.
This allows us to re-use the key pieces without being hooked into the `!$loki data` regions semantics.
Instead of subbing just on calls, we apply the remapping over the whole routine body.
Without tying the transformation to calls, explicit no-target skipping becomes virtually impossible; hence removing the test for it.
mlange05
force-pushed
the
naml-field-offload-regions
branch
from
82431ab to
cede76b
Compare
|
Ok, initial comments on this have been addressed and I've rebased this fairly recently. @awnawab Did you want to take a quick look over this before we merge it? |
|
Hi @mlange05. I'm happy to take a look at this, but I'll only be able to get to this closer to the end of play today. If you'd like it merged sooner than that then please go ahead. |
This PR refactors the current Field-API offload transformation and its utilities and adds two previously missing features:
CALL F(obj%a(:, 1), obj%a(:, 2))only declares and offloadsobj_aonce.FieldOffloadTransformationover compute regions in drivers without kernel callsMoreover, it turns I've started moving things around a little and ended up putting most of the heavy lifting in the
FieldPointerMaputility. To avoid future cyclic import and other issues, I've then moved this out to a pure helper sub-moduleloki.transformations.field_apiand renamed the using transformationsdata_offload.field_offloadandparallel.field_views.In refactoring the existing transformation and utilities to achieve the above, I moved quite a few things around - sorry for the large diff. I'd be happy to split this up, if this will aid the review process.
In a little more detail, the other changes in this PR are:
state_type, where appropriatedict.fromkeysinstead ofset, as this retains ordering and thus ensures reproducible orders of variables/utility calls at all stagesFieldPointerMapnot converts from field view array symbols to general data pointers (with block dimension) on-the-fly. This allows the data pointers to be given as a mere property on the pointer map object.FieldPointerMapnow provides the lists of boilerplate calls for host-to-device copies and host-sync calls as properties and derives them from the three internally stored argument/array symbol lists.